-
Notifications
You must be signed in to change notification settings - Fork 500
User schedule #2185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
User schedule #2185
Conversation
|
@differentreality thanks for taking care of this! 💘 |
|
Just a little lint to remove: |
|
I know it feels weird but try out code-review-by-commit. It makes things so much faster 🤠 |
7068bbc to
4610e9d
Compare
Codecov Report
@@ Coverage Diff @@
## master #2185 +/- ##
==========================================
+ Coverage 78.74% 82.01% +3.27%
==========================================
Files 146 146
Lines 4826 4798 -28
==========================================
+ Hits 3800 3935 +135
+ Misses 1026 863 -163
Continue to review full report at Codecov.
|
|
@differentreality once more! You can make it! ;-) |
4610e9d to
a287348
Compare
|
@differentreality: do you think you'll get this wrapped up soon? I'd love to have this in by mid-February, for LFNW2019! |
a287348 to
6317a52
Compare
|
Yes, I will! I had completely forgotten about this 😞 |
6317a52 to
455c98e
Compare
455c98e to
1111f6b
Compare
|
Rebased 😀 |
1111f6b to
e378956
Compare
|
What else would we need for this? |
e378956 to
717e487
Compare
|
Any chance of having this merged in soon-ish? We're hosting an event on Feb 6th and this would really help out! |
|
Yeah I'd love to see this merged as well! Still pending review though.. |
717e487 to
1e53e9a
Compare
|
@differentreality If I manually pull this commit into my local copy, I should just be able to migrate the database with the new relationship, right? The only failure in the CI build was a linter, so I'm not too worried...but I wanted to see what you thought first. |
cycomachead
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this idea!
I left some comments in terms of web accessibility with the front-end piece. Feel free to ignore it and just merge -- this seems great to me to have. (I'm hoping to make some small accessibility-related PRs soon.)
| - if current_user | ||
| = link_to('#', onClick: 'starClicked();') do | ||
| %span#star-events{ class: "fa fa-lg #{ event.favourite_users.exists?(current_user.id) ? 'fa-star' : 'fa-star-o' }", | | ||
| "aria-hidden" => "true", | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of accessibility, something like this would be provide a better description to anyone with a screen reader.
| "aria-hidden" => "true", | | |
| "aria-label" => "#{event.favourite_users.exists?(current_user.id) ? "un-" : ""}favourite event", | |
|
|
||
| - if current_user | ||
| = link_to('#', onClick: 'starClicked();') do | ||
| %span#star-events{ class: "fa fa-lg #{ event.favourite_users.exists?(current_user.id) ? 'fa-star' : 'fa-star-o' }", | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a class, not an id? (Since it seems like this partial is reusable)
|
|
||
| - if current_user | ||
| = link_to('#', onClick: 'starClicked();') do | ||
| %span#star{ class: "fa fa-lg #{ event.favourite_users.exists?(current_user.id) ? 'fa-star' : 'fa-star-o' }", | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, this should be a class, since it appears multiple times on the page.
| - if current_user | ||
| = link_to('#', onClick: 'starClicked();') do | ||
| %span#star{ class: "fa fa-lg #{ event.favourite_users.exists?(current_user.id) ? 'fa-star' : 'fa-star-o' }", | | ||
| "aria-hidden" =>"true", | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above. :)
| "aria-hidden" =>"true", | | |
| "aria-label" => "#{event.favourite_users.exists?(current_user.id) ? "un-" : ""}favourite event", | |
| colspan: span, | | ||
| role: "button" } | ||
| %a.unstyled-link{href: url_for(conference_program_proposal_path(@conference.short_title, event.id))} | ||
| %div{ onClick: 'eventClicked(event, this);', "data-url" => "#{url_for(conference_program_proposal_path(@conference.short_title, event.id))}" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of accessibility (both screen readers and keyboards), this is better as an a element. Otherwise, it needs role: 'button', tabindex: 0, onKeyPress: 'function(event) { if (event.charCode === 10 || event.charCode === 32) { eventCicked(event, this); } }' -- well, that last function is better put elsewhere, but that makes the div accept keypresses like a normal button.
| end | ||
|
|
||
| def toggle_favorite | ||
| user = User.find(params[:favourite_user_id]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've finally started working on merging this into my own fork for an event this summer. It seems to me that this allows anyone to toggle anyone else's favorite events. Should it not just use current_user? Since you should be logged in and have a cookie?
I can't really see a use case as well for someone else (such as an admin) toggling another user's favorites so I think the favourite_user_id parameter could probably just be removed, but I might be missing something.
| includes: [:room, { event: %i[track event_type speakers submitter] }] | ||
| ) | ||
|
|
||
| @events_schedules = @events_schedules.select{ |e| e.event.favourite_users.exists?(current_user.id) } if @events_schedules && current_user && @favourites |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be a slightly different use case, but I think related enough...
How would you feel about showing a schedule which is favorite events + events where you are speaking?
I am thinking about creating an event_schedule_for_user(user) method, which is essentially your personal conference agenda. My app has assigned volunteers which could also how up there.
77048df to
eebcf6b
Compare
| -if @events_schedules.any? | ||
| = render partial: 'schedule_tabs', locals: { active: 'program' } | ||
|
|
||
| %h1.text-center |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above line -if @events_schedules.any? probably should be removed if this is merged in. It leads to a weird looking case when you have no events in your favorites schedule and the tabs disappear.
Introduce an association between User and Event for the user schedule. As there was already a relation call users in Event the new relation is called public_users. And for the same reason the relation in Event is called public_events.
Make it possible for users to select events which they want to see in the public schedule and in the All events section of the public schedule.
Show the current user favourites events for both schedule and all events. Unsed css removed.
Introduce a separate action `toggle_favorite` to update user favorites events. Otherwise users with permissions to update the event can have it as favorite.
52eb61c to
efdb2cd
Compare

User can select favorite events, and show only their individual schedule
Continuing #1142 by @Ana06